Allow manual specification of python executable during build #1602

tguruswamy · 2024-01-08T00:25:41Z

common/configure and qt/configure now support --python=PYTHON_PATH option to manually specify the python executable. A full, absolute path is preferred but any valid path should work.

Previous --python and --python3 options also should still work, representing /usr/bin/python and /usr/bin/python3 respectively.

Tests pass on openSUSE Leap 15.5 and Tumbleweed.

This is the quick fix for #1574 .

…nd qt/configure

tguruswamy · 2024-01-08T02:33:48Z

I note that you can no longer change the python path twice by rerunning configure -- run it once only. But hopefully there isn't anyone relying on that.

buhtz · 2024-01-08T14:20:06Z

Thanks a lot for this contribution.
I am not into makefiles to review this PR.
Maybe we can ask on bit-dev for assistance?

aryoda · 2024-01-09T01:15:56Z

@buhtz I will reviewing and testing this in the next days

I note that you can no longer change the python path twice by rerunning configure -- run it once only.

Do you mean a 2nd ./configure does not change the Makefile's Python3? Strange, I will test this on Ubuntu...

Edit: OK, it is because sed does not find ^python3 anymore to replace it (once it has been changed).
This is IMHO a source of problems that are hard to find.
Perhaps there is a better solution that allows changing Python with every ./configure call.
@ALL Any proposals?

aryoda · 2024-01-09T01:31:21Z

common/configure

-esac
-sed -e "s/^python3\? /python${PYVERSION} /g" \
-    -e "s/^ssh-agent python3\? /ssh-agent python${PYVERSION} /g" \
+sed -e "s#^python3 #${PYTHON} #g" \


Why is # instead of / used as substitution delimiter?
It does work surprisingly...

Edit: man sed says:

cregexpc Match lines matching the regular expression regexp. The c may be any character.

so I would have expected a leading backslash...n

Indeed, any character can be used as long (as it doesn't appear in regexp, of course) -- I picked # since that was already used in qt/configure for the same purpose.

aryoda · 2024-01-09T01:38:15Z

common/configure

@@ -324,7 +328,7 @@ done

 # check python version
 PYTHON_VERSION_REQUIRED="3.8"
-PYTHON_VERSION_CURRENT=$(python${PYVERSION} --version | tr --delete 'Python ')
+PYTHON_VERSION_CURRENT=$(${PYTHON} --version | tr --delete 'Python ')


beautiful code now, well done 😃

tguruswamy · 2024-01-09T22:18:32Z

Edit: OK, it is because sed does not find ^python3 anymore to replace it (once it has been changed). This is IMHO a source of problems that are hard to find. Perhaps there is a better solution that allows changing Python with every ./configure call. @ALL Any proposals?

Yes, that's right. The proper way to do this (c.f. autotools) would be to have a template file ("backintime.in") which is used to create the real "backintime" every time configure is run. This would be a more invasive change, and may not be worth it to implement right now if you are rewriting the build system anyway.

aryoda · 2024-01-10T00:31:23Z

Yes, that's right. The proper way to do this (c.f. autotools) would be to have a template file ("backintime.in") which is used to create the real "backintime" every time configure is run. This would be a more invasive change, and may not be worth it to implement right now if you are rewriting the build system anyway.

I fully agree and given the fact that the python version is not changed on the build computer anymore (normally) it is IMHO a valid approach.

It would be great if you place just a short comment in the code as hint about this restriction. THX :-)

tguruswamy · 2024-01-10T02:19:01Z

Here's a compromise to avoid rewriting too much -- a version which prints a warning if the sed python path replacement fails. Feel free to change the text to something you like better.

I removed the ssh-agent pattern because I could not find anywhere in the code where it might apply.

buhtz · 2024-01-10T08:16:38Z

Dear @Fantu ,
as Debian co-maintainer can you please have a short look on this PR and tell is if it would negative influence the packaging process on your site?
Thanks in advance
Christian

aryoda · 2024-01-10T08:33:50Z

Here's a compromise to avoid rewriting too much -- a version which prints a warning if the sed python path replacement fails. Feel free to change the text to something you like better.

Perfect as is now!

I removed the ssh-agent pattern because I could not find anywhere in the code where it might apply.

Just checked it, you are right, ssh-agent is only called from within our Python source code so no Python version must be specified:

backintime/common/sshtools.py

Lines 255 to 305 in 2df7731

    
               def startSshAgent(self): 
        
                   """ 
        
                   Start a new ``ssh-agent`` if it is not already running. 
        
                   Raises: 
        
                       exceptions.MountException:  if starting ``ssh-agent`` failed 
        
                   """ 
        
                   SOCK = 'SSH_AUTH_SOCK' 
        
                   PID = 'SSH_AGENT_PID' 
        
                   if os.getenv(SOCK, '') and os.getenv(PID, ''): 
        
                       logger.debug( 
        
                           'ssh-agent already running. Skip starting a new one.', self) 
        
                       return 
        
                   sshAgent = tools.which('ssh-agent') 
        
                   if not sshAgent: 
        
                       raise MountException( 
        
                           _('ssh-agent not found. Please make sure it is installed.')) 
        
                   if isinstance(sshAgent, str): 
        
                       sshAgent = [sshAgent, ] 
        
                   sa = subprocess.Popen(sshAgent, 
        
                                         stdout=subprocess.PIPE, 
        
                                         stderr=subprocess.PIPE, 
        
                                         universal_newlines=True) 
        
                   out, err = sa.communicate() 
        
                   if sa.returncode: 
        
                       raise MountException( 
        
                           'Failed to start ssh-agent: [{}] {}' 
        
                           .format(sa.returncode, err)) 
        
                   m = re.match(r'.*{}(?:=|\s)([^;]+);.*{}(?:=|\s)(\d+);' 
        
                                .format(SOCK, PID), 
        
                                out, 
        
                                re.DOTALL | re.MULTILINE) 
        
                   if m: 
        
                       logger.debug('ssh-agent started successful: {}={} | {}={}' 
        
                                    .format(SOCK, m.group(1), PID, m.group(2)), self) 
        
                       os.environ[SOCK] = m.group(1) 
        
                       os.environ[PID] = m.group(2) 
        
                       atexit.register(os.kill, int(m.group(2)), signal.SIGKILL) 
        
                   else: 
        
                       raise MountException( 
        
                           'No matching output from ssh-agent: {} | {}'.format(out, err))

So this is useless legacy code (git blame dates this back to 2015)

Fantu · 2024-01-10T08:49:18Z

Dear @Fantu , as Debian co-maintainer can you please have a short look on this PR and tell is if it would negative influence the packaging process on your site? Thanks in advance Christian

Probably not, but I'm not sure.
You could compare the contents of the generated packages for example to see the differences, the latest version in Debian compared with the one generated by adding only this patch. Should be possible with debdiff.
I'm probably too busy in these days, if you can check it, if you have difficulty I'll try to do it when I can.

aryoda · 2024-01-15T00:26:53Z

@tguruswamy I have merged your PR now into our dev branch.
THX a lot for your excellent work helping to improve BiT! ❤️

Tejas Guruswamy and others added 4 commits January 7, 2024 17:42

Allow manual specification of python executable in common/configure a…

3f33c6f

…nd qt/configure

Add CHANGES entry

d5f1110

Should also work with no options provided

3395014

Need to use alternative sed delimiters for paths with /

305f0c6

buhtz requested a review from aryoda January 8, 2024 14:19

aryoda reviewed Jan 9, 2024

View reviewed changes

Print an error if no python path replacement is made

b708c97

aryoda approved these changes Jan 10, 2024

View reviewed changes

buhtz approved these changes Jan 10, 2024

View reviewed changes

aryoda merged commit 136f466 into bit-team:dev Jan 15, 2024
1 check failed

This was referenced Jan 21, 2024

backintime-qt script uses non-absolute python3 call #1316

Closed

BiT does not start after update: "No modul named 'dbus'" #1619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow manual specification of python executable during build #1602

Allow manual specification of python executable during build #1602

tguruswamy commented Jan 8, 2024

tguruswamy commented Jan 8, 2024

buhtz commented Jan 8, 2024

aryoda commented Jan 9, 2024 •

edited

Loading

aryoda Jan 9, 2024 •

edited

Loading

tguruswamy Jan 9, 2024

aryoda Jan 9, 2024

tguruswamy commented Jan 9, 2024 •

edited

Loading

aryoda commented Jan 10, 2024

tguruswamy commented Jan 10, 2024

buhtz commented Jan 10, 2024

aryoda commented Jan 10, 2024

Fantu commented Jan 10, 2024

aryoda commented Jan 15, 2024

Allow manual specification of python executable during build #1602

Allow manual specification of python executable during build #1602

Conversation

tguruswamy commented Jan 8, 2024

tguruswamy commented Jan 8, 2024

buhtz commented Jan 8, 2024

aryoda commented Jan 9, 2024 • edited Loading

aryoda Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

tguruswamy Jan 9, 2024

Choose a reason for hiding this comment

aryoda Jan 9, 2024

Choose a reason for hiding this comment

tguruswamy commented Jan 9, 2024 • edited Loading

aryoda commented Jan 10, 2024

tguruswamy commented Jan 10, 2024

buhtz commented Jan 10, 2024

aryoda commented Jan 10, 2024

Fantu commented Jan 10, 2024

aryoda commented Jan 15, 2024

aryoda commented Jan 9, 2024 •

edited

Loading

aryoda Jan 9, 2024 •

edited

Loading

tguruswamy commented Jan 9, 2024 •

edited

Loading